[IMP] queue_job: prevent commit during queue job execution#880
[IMP] queue_job: prevent commit during queue job execution#880
Conversation
|
Hi @guewen, |
sbidoul
force-pushed
the
19.0-prevent-commit-in-jobs
branch
from
03849bf to
6305ae3
Compare
|
As I understand, it is very likely to happen when using queue_job_cron, as a lot of cron methods do intermediate commits |
|
Precisely, yes. |
|
@guewen an alternative would be to acquire the job lock with a separate cursor, the drawback being doubling the number of postgres connections required for jobs. I'm not sure how prevalent the problem is, so to understand better I'm inclined to merge this so affected users get a clear error instead of hard to diagnose issues with job running multiple times. And if it appears that commit in jobs is a really really important use case then we can consider alternatives. |
Yes, even if crons/jobs are supposed to be idempotent, in case they are not, it can be devastating in some cases if they are run several times concurrently. A clear error is indeed much better than unexpected and erratic behavior. And it is not blocking anybody because if a cron job is flagged as "run as job" and raises "Commit is forbidden in queue jobs" because of a commit, they can still run it as a normal cron. Thinking about it, we may add some details in the error message, ex "Commit is forbidden in queue jobs. If the current job is a cron running as job, it should be run as a normal job"? |
sbidoul
force-pushed
the
19.0-prevent-commit-in-jobs
branch
from
6305ae3 to
eacc671
Compare
This would release the job lock, causing spurious restarts by the dead jobs requeuer.
sbidoul
force-pushed
the
19.0-prevent-commit-in-jobs
branch
from
eacc671 to
adc7d22
Compare
| job.store() | ||
| env.flush_all() | ||
| # TODO refactor, the relation between env and job.env is not clear | ||
| assert env.cr is job.env.cr |
There was a problem hiding this comment.
I initially was trying assert env is job.env but that fails. I don't quite understand why, but that's unrelated to this PR. What is important is that job.perform() uses the same transaction as the one that is commited with env.cr.commit() 5 lines below.
|
This is now ready. I implemented your error message suggestion and added a way to test this interactively. |
4 participants
Commiting in a job would release the job lock, causing spurious restarts by the dead jobs requeuer.
Very much draft and untested at this point.